Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Very generic input: Custom widget creator #1418

Merged

Conversation

ArturoManzoli
Copy link
Contributor

@ArturoManzoli ArturoManzoli commented Oct 18, 2024

Widget creation toolkit for parametric inputs and control via HTTP requests;

Screenshare.-.2024-10-18.4_09_58.PM.mp4
Screenshare.-.2024-10-18.4_15_54.PM.mp4

Closes #1291

@ArturoManzoli ArturoManzoli force-pushed the very-generic-input-widget branch 2 times, most recently from 8cb8e98 to 96ab58a Compare October 21, 2024 13:10
@ArturoManzoli
Copy link
Contributor Author

ArturoManzoli commented Oct 21, 2024

Last push: Fix dropdown selector not keeping last selected option after reload

@ArturoManzoli ArturoManzoli force-pushed the very-generic-input-widget branch from 96ab58a to 5cca995 Compare October 21, 2024 13:42
Copy link
Member

@rafaellehmkuhl rafaellehmkuhl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. The horizontal edit-menu bar with the custom widget options does not have a consistent scroll with respect to the other two menus (regular and mini).

Testes on macOS. Vertically scrolling the bar should scroll horizontally.

  1. This is breaking the edit-menu widget container width again:
image
  1. Does any problem arise if we allow the user to add the custom input widgets to the mini-widget containers? This would make them ever more powerful! This way they can have them on the top/bottom bar.

  2. Should we have this as a button? Is there any other usage that is not tied to a Cockpit Action Variable?

image
  1. The slider on "large" size is overflowing the container and the label as well. Increasing the container size does not help:
image
  1. The changes are using "Cockpit Action Parameter" instead of the new name "Cockpit Action Variable".

Copy link
Member

@rafaellehmkuhl rafaellehmkuhl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question: should we indeed separate the whole pipeline for the custom widgets from the mini-widgets?

I understand having them on a separate dropdown on the edit-menu, to make it easier for the user to understand, but I couldn't see a good reason to separate their container from the mini-widgets. Being able to mix them feels like a big feature to me, and if we separate them now, it will be difficult to merge them later.

everMounted: false,
configMenuOpen: false,
highlighted: false,
customWidgetVars: CustomWidgetVarsType.Button,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we call it customWidgetType?

Comment on lines 523 to 532
:ref="(el) => (miniWidgetContainers[miniWidget.component] = el as HTMLElement)"
:key="miniWidget.hash"
class="flex flex-col items-center justify-between w-full rounded-md bg-[#273842] hover:brightness-125 h-[90%] aspect-square cursor-pointer elevation-4 overflow-clip pointer-events-none"
class="flex flex-col items-center justify-between rounded-md bg-[#273842] hover:brightness-125 h-[90%] aspect-square cursor-pointer elevation-4 overflow-clip pointer-events-none"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the part that breaks the auto-width.

Copy link
Contributor Author

@ArturoManzoli ArturoManzoli Oct 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it is and it's weird how this zombie change keep being deleted and brought back to life over and over... 😄

<div class="flex items-center justify-center w-full p-1 transition-all bg-[#3B78A8] rounded-b-md text-white">
<span class="whitespace-normal text-center">{{
element.name.replace(/([a-z])([A-Z])/g, '$1 $2').replace(/^./, (str) => str.toUpperCase()) ||
'Very Generic Indicator'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code is turning a camelCase name such as RelativeAltitudeIndicator to Relative Altitude Indicator to fit the card title. But the exception is indeed unnecessary.

Comment on lines 35 to 61
type ElementType = {
/**
*
*/
position: {
/**
*
*/
x: number
/**
*
*/
y: number
}
/**
*
*/
size: {
/**
*
*/
width: number
/**
*
*/
height: number
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blank jsdocs.

Comment on lines 466 to 467
<div
v-show="widgetMode === 'Regular widgets'"
class="2xl:text-md text-sm mt-3 2xl:px-3 px-2 bg-[#3B78A8] rounded-lg"
>
Click or drag to add
<div v-show="widgetMode === 'Regular'" class="2xl:text-md text-sm mt-3 2xl:px-3 px-2 rounded-lg">
(Click on card to add)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we are wrongly removing a functionality here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We did rename the selector so it wont have the word 'widgets' repeated over and over. But the helper text should have remained the same.

Copy link
Member

@rafaellehmkuhl rafaellehmkuhl Oct 31, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean the "Click or drag to add" vs "Click to add".

width: `${100 * size.value.width}%`,
height: `${100 * size.value.height}%`,
width: disableResponsiveness.value ? `${1000 * size.value.width}px` : `${100 * size.value.width}%`,
height: disableResponsiveness.value ? `${1000 * size.value.height}px` : `${100 * size.value.height}%`,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Curiosity: what was the motive for that the custom widgets to be fundamentally different in the resize behavior? I found it to be counterintuitive to the user.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There was a concern about how the various input mini-widgets would react to the custom-widget-base responsiveness. Generic widgets shrink with the window size and its contents are scaled down too.
But this shrink behavior wasn't ideal for input components.

This prop was also causing some size issues on a recently added widget base.
I'll remove it for now and let's see how the responsiveness is doing on the custom widgets.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you forgot to remove this (and the other mentions to disableResponsiveness along the code).

@ArturoManzoli
Copy link
Contributor Author

Question: should we indeed separate the whole pipeline for the custom widgets from the mini-widgets?

I understand having them on a separate dropdown on the edit-menu, to make it easier for the user to understand, but I couldn't see a good reason to separate their container from the mini-widgets. Being able to mix them feels like a big feature to me, and if we separate them now, it will be difficult to merge them later.

Totally agree. Allowing the user to add custom mini-widgets to the top, bottom bar and to any other future uses for mini widgets will indeed make Cockpit a lot more powerful.

@ArturoManzoli
Copy link
Contributor Author

4. Should we have this as a button? Is there any other usage that is not tied to a Cockpit Action Variable?

Can you give more info about it? Maybe an example of another usage so we decide if we should implement now or on the next functionalities update

@ArturoManzoli ArturoManzoli force-pushed the very-generic-input-widget branch from 798d9c5 to c53b578 Compare October 30, 2024 13:09
@ArturoManzoli
Copy link
Contributor Author

ArturoManzoli commented Oct 30, 2024

On last push:

  • Made the changes requested by @rafaellehmkuhl;
  • Custom mini widgets are now called 'input' widgets and are now compatible with the regular mini widgets. They all can be placed alongside each other, on the top and bottom bar, and inside user's custom widgets. (video below)
  • All mini widgets are now instantiated by the same wrapper (MiniWidgetInstatiator.vue)
  • Still the issue of the card being dragged with the mini widget. This will be solved on a separate task;
Screenshare.-.2024-10-30.9_57_31.AM.mp4

@rafaellehmkuhl
Copy link
Member

  1. Should we have this as a button? Is there any other usage that is not tied to a Cockpit Action Variable?

Can you give more info about it? Maybe an example of another usage so we decide if we should implement now or on the next functionalities update

My point is that all input widgets are being created to inject variables in the data-lake, and having a button there means creating this variable is optional, when it's in fact mandatory. Those should be required fields.

@rafaellehmkuhl
Copy link
Member

I'm not able to config the input widgets with the last PR. It was working fine in the previous version thought:

Kapture.2024-11-01.at.06.42.32.mp4

@ArturoManzoli
Copy link
Contributor Author

ArturoManzoli commented Nov 1, 2024

I'm not able to config the input widgets with the last PR. It was working fine in the previous version thought:

This is something related to the instantiation of the input widget on the top and bottom bar. If you refresh the screen the config panel will open normally.

Will fix that

@ArturoManzoli
Copy link
Contributor Author

ArturoManzoli commented Nov 1, 2024

  1. Should we have this as a button? Is there any other usage that is not tied to a Cockpit Action Variable?

Can you give more info about it? Maybe an example of another usage so we decide if we should implement now or on the next functionalities update

My point is that all input widgets are being created to inject variables in the data-lake, and having a button there means creating this variable is optional, when it's in fact mandatory. Those should be required fields.

I understand you point that the variable name is mandatory to inject data, but if the user is, at the moment, just creating the custom widget and arranging its layout? Maybe it doesn't need to be named/registered at that very moment, and the user (or even someone else) can do this data injection config on a later moment.

The blue 'create' button was thought the be spotted right away by the user and probably wont be missed out. Also, the whole process for controlling something by the input widgets will usually need some documentation reading/video tutorial for the user to fully understand its applications and usage. So, the variable creation will be something internalized by the user, I suppose.

Also, every variable injected on the data lake will take a small amount of system resources to be kept tracked or on listening mode.

Let me know your thoughts about this.

Anyway, I modified the workflow, so when an input widget is placed on the top or bottom bar, its config panel will always pop out. This way, the user will be 'invited' to configure its proprieties right away

@ArturoManzoli ArturoManzoli force-pushed the very-generic-input-widget branch from c53b578 to a1233c5 Compare November 1, 2024 11:44
@rafaellehmkuhl
Copy link
Member

@ArturoManzoli it does make sense!

Copy link
Member

@rafaellehmkuhl rafaellehmkuhl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm trying to setup a button widget but it's not calling the Cockpit Action it was assigned to:

Kapture.2024-11-04.at.23.52.19.mp4

@@ -596,28 +639,52 @@ import { computed, onMounted, ref, toRefs, watch } from 'vue'
import { nextTick } from 'vue'
import { type UseDraggableOptions, useDraggable, VueDraggable } from 'vue-draggable-plus'
import { defaultMiniWidgetManagerVars } from '@/assets/defaults'
import { defaultCustomWidgetManagerVars, defaultMiniWidgetManagerVars } from '@/assets/defaults'
/* @ts-ignore */
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Those @ts-ignore are not needed. You can remove them and push and will see that the CI will not complaint.

Comment on lines 536 to 565
miniWidget.name.replace(/([a-z])([A-Z])/g, '$1 $2').replace(/^./, (str) => str.toUpperCase()) ||
'Very Generic Indicator'
miniWidget.name.replace(/([a-z])([A-Z])/g, '$1 $2').replace(/^./, (str) => str.toUpperCase())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if it's because this specific change, but the VGI name disappeared:

image

Comment on lines 12 to 17
* Mini-widget instance
*
*/
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to remove this line.

width: `${100 * size.value.width}%`,
height: `${100 * size.value.height}%`,
width: disableResponsiveness.value ? `${1000 * size.value.width}px` : `${100 * size.value.width}%`,
height: disableResponsiveness.value ? `${1000 * size.value.height}px` : `${100 * size.value.height}%`,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you forgot to remove this (and the other mentions to disableResponsiveness along the code).

src/components/mini-widgets/ArmerButton.vue Show resolved Hide resolved
Comment on lines 36 to 46
throw new Error(`Cockpit action variable with id '${variable.id}' already exists. Update it instead.`)
console.log(`Cockpit action variable with id '${variable.id}' already exists. Renaming to ${variable.name}_new.`)
showSnackbar({
message: `Cockpit action variable with id '${variable.id}' already exists. Renaming to ${variable.name}_new.`,
})
variable.id = `${variable.id}_new`
variable.name = `${variable.name}_new`
cockpitActionVariableInfo[variable.id] = variable
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's no need for the snackbar here.
You can have the snackbar in the component that is calling createCockpitActionVariable.
This is a void method with a throw, so there you can put a try/catch block to deal with error/success case, like so:

// EditMenu.vue

import { useSnackbar } from '@/composables/snackbar'

const { showSnackbar } = useSnackbar()

try {
  createCockpitActionVariable(variable)
  showSnackbar({ message: 'Parameter successfully updated', variant: 'success' })
} catch(error) {
  showSnackbar({message: `Failed to add Cockpit action variable with id '${variable.id}'. Name already exists. Please rename it.`})
}

If necessary to deal with the renaming process, please do it in the EditMenu.vue component. You can do that by adding an code property to the error (inside the data-lake.ts file) and checking for the error in the Vue component, although I think the more correct approach is to ask the user to rename the variable, so it's clear to them that they already have a variable with that name).

@@ -312,7 +312,7 @@
</template>

<script setup lang="ts">
import { computed, onMounted, reactive, ref } from 'vue'
import { computed, onMounted, reactive, ref, watch } from 'vue'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unnecessary change. Should be removed.

@@ -665,6 +665,7 @@ const saveUrlParameter = (): void => {

onMounted(() => {
loadSavedActions()
console.log('🚀 ~ allSavedActionConfigs:', allSavedActionConfigs.value)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unnecessary change. Should be removed.

src/components/MiniWidgetInstantiator.vue Outdated Show resolved Hide resolved
src/components/WidgetHugger.vue Show resolved Hide resolved
Copy link
Member

@rafaellehmkuhl rafaellehmkuhl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code-wise I think everything is fine now.
The button is also working (for calling actions).
What I can't get to work are the inputs. I tried to add a slider input, but it isn't updating it's action variable. It keeps undefined, on both boot and while moving it. The test below has both a setInterval as well as a listenCockpitActionVariable added to the code, and none get any change in the slider value:

Kapture.2024-11-11.at.06.57.12.mp4

@ArturoManzoli ArturoManzoli force-pushed the very-generic-input-widget branch 2 times, most recently from a6adc55 to 6709204 Compare November 11, 2024 10:51
@rafaellehmkuhl
Copy link
Member

We discussed in private and came to the conclusion that there was a missing function in the data-lake for those to work as expected. The ability to set the initial value was implemented on #1443 and will be used here.

@ArturoManzoli ArturoManzoli force-pushed the very-generic-input-widget branch from 6709204 to fb1ce39 Compare November 11, 2024 21:05
@ArturoManzoli ArturoManzoli force-pushed the very-generic-input-widget branch 2 times, most recently from f84cbd5 to eb882a2 Compare November 18, 2024 12:22
Copy link
Member

@rafaellehmkuhl rafaellehmkuhl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ArturoManzoli I've found some problems in my last test. Will list them below:

  1. I'm testing here but I'm having the same NaN problem with the Dial widget I was having in our last talk. Was this part worked on?
Kapture.2024-11-19.at.08.52.24.mp4

I made sure I was using the latest branch version for the test. Vite server was reloaded and application process was killed and reset.

I could reproduce it with both old Dial widgets as well as to ones I had just added. The problem happens if you add a Dial widget, create a data-lake variable to it but reload the application without using the dial for the first time before the reload. I can reproduce it 100% of the times I try. If I use the dial (turn it) before reloading Cockpit, the problem does not happen, but if I don't, the Dial gets stuck at NaN forever.

  1. When I drag an input widget from the CustomWidgetBase, the trash icon does not appear.

Also around that, the edit-menu left panel is not showing the CustomWidgetBase as a container for mini-widget, so that the user can delete or configure mini-widgets from there, although this is a problem also for the current MiniWidgetBar widget, so I opened issue #1452 to track that. Feel free to work on it on this PR or to let it as it is and we fix it later for both.

  1. Both the switch as well as the checkbox, when with a false value, require two clicks to change to true again.

Comment on lines 64 to 72
export enum CustomWidgetType {
Button = 'boolean',
Checkbox = 'boolean',
Dial = 'number',
Dropdown = 'string',
Label = 'string',
Slider = 'number',
Switch = 'boolean',
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was created but was never used.

/**
* Variable type
*/
variableType: string
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checkbox is not of type string.
Actually all elements are set as string type.

/**
* Last known value of the element
*/
lastValue: string | number | boolean | undefined
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Last value here should be boolean or undefined, not string or number.

@ArturoManzoli ArturoManzoli force-pushed the very-generic-input-widget branch from eb882a2 to c722735 Compare November 21, 2024 10:35
@ArturoManzoli
Copy link
Contributor Author

@rafaellehmkuhl, I had to relocate the lastValue storage location out of the mini-widget data object due to reactivity problems on some boolean vuetify components. Its working fast and reliable as I tested. Take a look and see if we need to store those values somewhere else.

@rafaellehmkuhl
Copy link
Member

@rafaellehmkuhl, I had to relocate the lastValue storage location out of the mini-widget data object due to reactivity problems on some boolean vuetify components. Its working fast and reliable as I tested. Take a look and see if we need to store those values somewhere else.

I feel like those changes are passing a lot of the responsibility that should be in the widgets themselves to the widget management store, but at the same time I don't want to postpone the merging of this feature much more.

I will perform some more tests to see if everything is working and take a look at the code again, and if everything goes right we are good to go.

const isElementsPropsDrawerVisible = ref(false)
const elementToShowOnDrawer = ref<CustomWidgetElement>()
const widgetToEdit = ref<Widget>()
const miniWidgetLastValues = useBlueOsStorage<Record<string, any>>('mini-widget-last-values', {})
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One small thing here: the storage key should start with "cockpit-...".

Copy link
Contributor Author

@ArturoManzoli ArturoManzoli Nov 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. I also have relocated the save widget method to outside the store

Copy link
Member

@rafaellehmkuhl rafaellehmkuhl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review points raised on a private meeting:

  • Trash can does not appear for widgets in the custom-widgets-base
  • Right configuration panel does not appear for widgets in the custom-widgets-base
  • Dial shows an integer but is generating a floating point number
  • Some input widgets are not specifying its types in the data-lake
  • Delete button in the input widgets sometimes does not work
  • Input widgets in the custom-widgets-base can get out of the container (hidden)

@ArturoManzoli ArturoManzoli force-pushed the very-generic-input-widget branch 3 times, most recently from fadfdb9 to dd6e29c Compare November 25, 2024 16:47
@ArturoManzoli
Copy link
Contributor Author

  • Input widgets in the custom-widgets-base can get out of the container (hidden)

This will depend on how the user resizes the custom widget, or how it responds to screen size changes.
There is no limit on how many parallel mini widgets the user can, so its possible to the user to configure a very wide but short custom widget.

@ArturoManzoli ArturoManzoli force-pushed the very-generic-input-widget branch 2 times, most recently from 2e9c060 to a8fcf34 Compare November 27, 2024 20:28
@ArturoManzoli ArturoManzoli force-pushed the very-generic-input-widget branch from a8fcf34 to ae3565a Compare November 27, 2024 20:35
@ArturoManzoli ArturoManzoli force-pushed the very-generic-input-widget branch from ae3565a to afb250f Compare November 27, 2024 20:38
@ArturoManzoli ArturoManzoli requested review from rafaellehmkuhl and removed request for rafaellehmkuhl November 27, 2024 20:45
Copy link
Member

@rafaellehmkuhl rafaellehmkuhl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work!

This is such a great feature and I expect it to be on the core usage of Cockpit!

There's a small bug, but I consider it to be an edge-cases, and I think this is such a cool feature that we don't need to hold this PR for it, so I opened issue #1479 for it and will be merging this PR now!

Have a great weekend!

@rafaellehmkuhl rafaellehmkuhl merged commit bba019e into bluerobotics:master Nov 29, 2024
10 checks passed
@ES-Alexander ES-Alexander added the docs-needed Change needs to be documented label Nov 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs-needed Change needs to be documented
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cockpit needs "input" widgets
3 participants